[flang] Fix build on different of cores from #164630 #164841
Conversation
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Jean-Didier PAILLEUX (JDPailleux) ChangesNormally fix incorrect linking introduced in #161179 with build in parallel. Full diff: https://github.com/llvm/llvm-project/pull/164841.diff 1 Files Affected:
diff --git a/flang/lib/Optimizer/Dialect/MIF/CMakeLists.txt b/flang/lib/Optimizer/Dialect/MIF/CMakeLists.txt
index ed8463e9b0330..d53937ebb49d4 100644
--- a/flang/lib/Optimizer/Dialect/MIF/CMakeLists.txt
+++ b/flang/lib/Optimizer/Dialect/MIF/CMakeLists.txt
@@ -8,7 +8,6 @@ add_flang_library(MIFDialect
LINK_LIBS
FIRDialect
FIRDialectSupport
- FIRSupport
LINK_COMPONENTS
AsmParser
|
|
This PR looks weird (removing things rather than adding to the CMakefile), but somehow it made our CI green. Note that I had to test for the other issue alongside, so it wasn't the recommended 'change one thing at a time' approach. |
There was a problem hiding this comment.
Thank you for taking time for looking into this but I don't think this is the whole right fix yet. The error message in #164630 would indicate a missing dependency between whatever is including MIFDialect.h and the CMake task which is generating the table gen parts of that header (MIFDialect).
I had a quick look and I see that the header file is included from flang/include/flang/Optimizer/Support/InitFIR.h. FIRSupport has no dependency upon MIFDialect and so there is no guarantee that it will be built after MIFDialect.
I think removing the dependency from MIFDialect to FIRSupport makes the issue much less likely (because CMake won't be trying to build FIRSupport before it has built MIFDialect) but it doesn't guarantee that the build order will be correct in the future for all machines.
Beware that the dependency is created by the presence of the include in a header (InitFIR.h) and so every library that includes InitFIR.h will also depend upon MIFDialect. However, they should already have a dependency on FIRSupport so it should come through transitively once FIRSupport has the right dependency.
I hope that made sense. Please feel free to ask me questions here or on the flang slack if I can help at all :)
|
That was my conclusion. I was unable to reproduce the dependency issue on my side. |
Seems like flang suffers from a major design issue when it comes to specifying dependencies. |
This isn't a flang-specific design decision it is how MLIR dialects work - and I'm not sure using tablegen to create header files in LLVM is a bad design decision. However I agree that we would benefit from some tooling that verified these dependency graphs. |
|
I added a dependency to MIFDialect in FIRSupport as discussed with @tblah |
So in the end, it's |
|
Yes, that's right. I don't know why I added this dependency (perhaps an artifact from my first try). |
4 participants
Normally fix incorrect linking introduced in #161179 with build in parallel.